Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SDL2 thread proxying fixes #5365

Merged
merged 5 commits into from
Mar 31, 2022
Merged

SDL2 thread proxying fixes #5365

merged 5 commits into from
Mar 31, 2022

Conversation

NeroBurner
Copy link
Contributor

@NeroBurner NeroBurner commented Feb 24, 2022 •
edited
Loading

Description

This PR uses new APIs added in emscripten-core/emscripten#9336 to improve compatibility with USE_PTHREADS=1.

Original PR: emscripten-ports/SDL2#127
By: @jakogut
Reviewed by: @Daft-Freak

This fix enables me to compile an executable using -s USE_PTHREADS=1 -s USE_SDL=2 and not get the following error in the chromuim debug console

Uncought TypeError: Cannot read properties of undefined (reading 'CreateImageData')

Uncought TypeError CreateImageData

With the current patch applied the InfiniSim Simulator works 🎉

InfiniSim wasm working

Existing Issue(s)

@NeroBurner
SDL2 thread proxying fixes
ed77e0f 
This PR uses new APIs added in [emscripten-core/emscripten#9336](emscripten-core/emscripten#9336)
to improve compatibility with USE_PTHREADS=1.

Original PR: emscripten-ports/SDL2#127
By: @jakogut
Reviewed by: Daft-Freak
@NeroBurner NeroBurner mentioned this pull request Feb 24, 2022
Draft
@Daft-Freak
Copy link
Collaborator

I think some of the comments from the original PR still apply, though I still haven't really looked into emscripten's threading stuff...

Daft-Freak
Daft-Freak reviewed Feb 25, 2022
View reviewed changes
Copy link
Collaborator

@Daft-Freak Daft-Freak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor cast warning and formatting comments. Also SDL_GetDisplayUsableBounds has some more EM_ASMs that were added after the original PR.

src/video/emscripten/SDL_emscriptenmouse.c Outdated Show resolved Hide resolved
src/video/emscripten/SDL_emscriptenmouse.c Outdated Show resolved Hide resolved
src/video/emscripten/SDL_emscriptenmouse.c Show resolved Hide resolved
src/video/emscripten/SDL_emscriptenframebuffer.c Outdated Show resolved Hide resolved
@NeroBurner
Copy link
Contributor Author

I hope I addressed the required changes. Can't follow about SDL_GetDisplayUsableBounds as I don't think I've touched that in this PR. Is there something I should update anyways? Just trying to "champion" these changes, as they solve an obstacle for me.

@slouken slouken requested a review from icculus February 26, 2022 20:42
@slouken slouken added this to the 2.0.22 milestone Feb 26, 2022
@uyjulian
Copy link
Contributor

uyjulian commented Mar 6, 2022

Yeah you should update SDL_GetDisplayUsableBounds to also proxy to main thread

@icculus
Copy link
Collaborator

icculus commented Mar 13, 2022

@Daft-Freak Do we want to merge this and deal with SDL_GetDisplayUsableBounds separately, or should one of us add it to this PR?

@Daft-Freak
Copy link
Collaborator

It's probably just EM_ASM_INT_V -> MAIN_THREAD_EM_ASM_INT there too, I haven't got around to trying it though. Okay with merging as-is or with the extra change if someone tests it.

@icculus icculus merged commit fe79eb2 into libsdl-org:main Mar 31, 2022
@icculus
Copy link
Collaborator

icculus commented Mar 31, 2022

I added the SDL_GetDisplayDisplayBounds stuff in ea7d530, so this is good to go now.

@NeroBurner
Copy link
Contributor Author

thanks a lot @icculus 🙇‍♂️

@NeroBurner NeroBurner deleted the emscripten_pthread_fix branch March 31, 2022 18:17
@connorjclark connorjclark mentioned this pull request Apr 12, 2022
Open
@Daft-Freak Daft-Freak mentioned this pull request May 8, 2022
Merged
@connorjclark connorjclark mentioned this pull request Jun 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Daft-Freak Daft-Freak Daft-Freak left review comments

@icculus icculus icculus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0.22
Development

Successfully merging this pull request may close these issues.
5 participants
@NeroBurner @Daft-Freak @uyjulian @icculus @slouken